Repository navigation
refactor: streamline dependencies and add tests - #1
Merged
Merged
Conversation
- remove inline package installation and loading from R modules - use fully qualified function names and native pipe syntax - add helpers.R with centralized visualization font setup - introduce run_tests.R with coverage for stats and charts - update CI package list and README requirements Signed-off-by: mingcheng <mingcheng@apache.org>
Reviewer's GuideCentralizes dependency management and visualization font setup, replaces inline package loading with namespace-qualified calls and native pipes, and adds a small test harness plus CI integration to validate stats aggregation and chart generation. Sequence diagram for shared visualization font setupsequenceDiagram
participant main_R as main.R
participant helpers_R as helpers.R
participant pie_R as pie.R
participant treemap_R as treemap.R
main_R->>helpers_R: source_module R/helpers.R
main_R->>pie_R: source_module R/pie.R
main_R->>treemap_R: source_module R/treemap.R
main_R->>pie_R: generate_pie_chart(username, repos, dir_name, json_data)
pie_R->>helpers_R: setup_visualization_font(font_path)
helpers_R-->>pie_R: font_family
pie_R->>pie_R: generate_chart(..., family = font_family)
main_R->>treemap_R: generate_treemap(username, repos, dir_name, json_data)
treemap_R->>helpers_R: setup_visualization_font(font_path)
helpers_R-->>treemap_R: font_family
treemap_R->>treemap_R: ggplot2::ggplot(..., theme_minimal(base_family = font_family))
Flow diagram for centralized package checking and module sourcingflowchart TD
A[main.R start] --> B[Define required_packages]
B --> C[Compute missing_packages with requireNamespace]
C -->|missing_packages length > 0| D[stop with install.packages message]
C -->|no missing packages| E[source_module]
E --> F[lapply over R/helpers.R, R/fetch.R, R/plot.R, R/pie.R, R/treemap.R]
F --> G[fetch_json and plot_repos]
G --> H[generate_pie_chart]
G --> I[generate_treemap]
H --> J[Pie chart files]
I --> K[Treemap files]
J --> L[Analysis complete]
K --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR centralizes R dependency and module loading, adds shared visualization font handling, updates plotting code to use explicit namespaces, introduces chart and aggregation tests, and integrates test execution into CI and README instructions. ChangesR workflow and visualization updates
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/r.ymlTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR refactors the R modules to remove inline package installation/loading, centralizes visualization font setup, and adds a lightweight Rscript-based test runner that is executed in CI.
Changes:
- Centralize custom font registration and
showtextactivation inR/helpers.R, and update chart modules to use it. - Refactor R modules to use namespaced package calls and the native pipe operator (
|>), with dependency checks moved toR/main.R. - Add
tests/run_tests.R, wire it into GitHub Actions, and document test execution in the README.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/run_tests.R | Adds a simple test runner that parses R files, validates aggregation output, and checks chart output generation. |
| README.md | Documents test execution and updates the required package list / install command. |
| R/treemap.R | Removes inline package management, uses namespaced ggplot2/treemapify calls, and uses centralized font setup. |
| R/plot.R | Removes inline package management and switches dplyr pipeline to native ` |
| R/pie.R | Removes inline font/package setup and uses centralized font setup; standardizes output naming using today. |
| R/main.R | Adds upfront required-package validation and a common module loader; uses namespaced jsonlite functions. |
| R/helpers.R | Introduces a shared helper to register the bundled font and enable showtext with fallback behavior. |
| R/fmt.R | Changes formatting script to fail fast when styler is missing (no implicit install). |
| R/fetch.R | Removes inline installs/library calls and uses fully qualified httr/jsonlite calls. |
| .github/workflows/r.yml | Updates package list, adjusts install dependencies mode, and runs the new tests before analysis. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
69
to
+70
| - R >= 4 (tested on 4.5.1) | ||
| - R packages: `httr`, `jsonlite`, `dplyr`, `magrittr`, `showtext`, `ggplot2`, `treemapify`, `svglite` | ||
| - R packages: `httr`, `jsonlite`, `dplyr`, `showtext`, `sysfonts`, `ggplot2`, `treemapify`, `svglite` |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/r.yml:
- Line 36: Update the path filters in the workflow so changes under assets/**,
including assets/FiraCode.ttf, trigger the test suite alongside the existing R
and tests/** patterns.
In `@README.md`:
- Around line 70-74: Update the documented minimum R version in README.md from R
>= 4 to R >= 4.1, ensuring the installation requirements accurately reflect the
native |> syntax used by R/plot.R.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dbd77c6-ffb5-4db4-8c8c-e1a83c2d8b61
📒 Files selected for processing (10)
.github/workflows/r.ymlR/fetch.RR/fmt.RR/helpers.RR/main.RR/pie.RR/plot.RR/treemap.RREADME.mdtests/run_tests.R
| - main | ||
| paths: | ||
| - "R/**" | ||
| - "tests/**" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trigger validation when the bundled font changes.
The test suite depends on assets/FiraCode.ttf, but this path filter covers R and test files only. Add assets/** so a removed or invalid font runs CI.
Proposed fix
- "tests/**"
+ - "assets/**"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - "tests/**" | |
| - "tests/**" | |
| - "assets/**" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/r.yml at line 36, Update the path filters in the workflow
so changes under assets/**, including assets/FiraCode.ttf, trigger the test
suite alongside the existing R and tests/** patterns.
Comment on lines
+70
to
+74
| - R packages: `httr`, `jsonlite`, `dplyr`, `showtext`, `sysfonts`, `ggplot2`, `treemapify`, `svglite` | ||
|
|
||
| ```r | ||
| install.packages(c("httr", "jsonlite", "dplyr", "showtext", "sysfonts", "ggplot2", "treemapify", "svglite")) | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n '\|>' R
rg -n 'R (>=|version)|r-version|R_VERSION' README.md .githubRepository: mingcheng/langstat
Length of output: 489
🌐 Web query:
R 4.0.0 release notes native pipe |><- operator introduced version
💡 Result:
The native pipe operator |> was not introduced in R 4.0.0; it was introduced in R 4.1.0 [1][2][3][4]. While R 4.0.0 included various updates and changes, it did not include the |> operator [5]. The native forward pipe syntax |> was officially added in R 4.1.0, allowing for the piping of the left-hand side expression as the first argument into the right-hand side call [3][6][7]. Subsequent versions of R expanded the functionality of the native pipe, such as the addition of the _ placeholder in R 4.2.0 [6].
Citations:
- 1: https://stat.ethz.ch/R-manual/R-devel/library/base/html/pipeOp.html
- 2: https://github.com/wch/r-source/blob/trunk/src/library/base/man/pipeOp.Rd
- 3: https://opensource.posit.co/blog/2023-04-21_base-vs-magrittr-pipe/
- 4: https://r-statistics.co/R-Pipe-Operator-in-R.html
- 5: https://cran.r-project.org/bin/windows/base/old/4.0.0/NEWS.R-4.0.0.html
- 6: https://tidyverse.org/blog/2023/04/base-vs-magrittr-pipe/
- 7: https://github.com/wch/r-source/blob/eab08ba9061594af466a8854b8158cdc83342a40/doc/NEWS.Rd
Raise the minimum documented R version to 4.1.
The project uses native |> syntax in R/plot.R, which R 4.0 cannot parse. Update README.md from R >= 4 to R >= 4.1 before users on the documented minimum try to install/run the package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 70 - 74, Update the documented minimum R version in
README.md from R >= 4 to R >= 4.1, ensuring the installation requirements
accurately reflect the native |> syntax used by R/plot.R.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Centralize dependency management and visualization font handling while adding basic test coverage for statistics and chart generation.
New Features:
Enhancements:
CI:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation